Add default database settting, and env var overrides #73
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
requested review from
tnull
and removed request for
benthecarman and
tnull
tankyleo
force-pushed
the
default-db-config
branch
from
485243d to
5f38982
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @tnull! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @tnull! This PR has been waiting for your review. |
tankyleo
force-pushed
the
default-db-config
branch
from
5f38982 to
1c1500f
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, some minor comments.
rust/server/vss-server-config.toml
Outdated
| host = "localhost" | ||
| port = 5432 | ||
| database = "postgres" | ||
| default_database = "postgres" |
There was a problem hiding this comment.
Might make sense to add comment here to specify that this will only be used for the initial connection. Otherwise it could be confusing why the user needs to specify two separate databases.
|
|
||
| format!("postgresql://{}:{}@{}:{}", username, password, self.host, self.port) | ||
| } | ||
| pub(crate) struct Configuration { |
There was a problem hiding this comment.
So now we have Config and Configuration? That seems rather confusing. Can they be merged, or at least one renamed appropriately to better distinguish their intended use?
rust/server/src/util/config.rs
Outdated
| toml::from_str(&config_file) | ||
| .map_err(|e| format!("Failed to parse configuration file: {}", e))?; | ||
|
|
||
| macro_rules! read_env { |
There was a problem hiding this comment.
Do we really need macros for this? Maybe we can inline these or make them methods?
|
Thanks tnull addressed your comments let me know if I can rebase |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Thanks tnull addressed your comments let me know if I can rebase
Please go ahead and squash & rebase. While you're at it, you might address the one nit below.
rust/server/src/util/config.rs
Outdated
| // environment variable. | ||
| #[derive(Deserialize, Default)] | ||
| struct Config { | ||
| struct TomlConfigTable { |
There was a problem hiding this comment.
nit: When you squash, can we drop the Table here?
Allow only a subset of `JwtAuthConfig` fields to be set in config file, as we allow any members of the configuration tables to not be set in the file, and instead be set by the corresponding environment variable.
...just like we do for the RSA public key used to authenticate incoming JWTs. Also parse the root certificate in `PostgresTlsBackend::new`. This makes `PostgresTlsBackend` consistent with `JWTAuthorizer`; both structs take cryptographic objects as `&str`'s and parse them in their constructors.
Not all postgres hosted services use `postgres` Also rename `database` config parameter to `vss_database`
This allows users to configure VSS-Server entirely through environment variables, without a configuration file. We hence remove the requirement that a path to a configuration file always be passed as an argument. If a file is passed as an argument, and it does not exist, we now abort startup. Also consolidate host and port settings into single socket address setting.
tankyleo
force-pushed
the
default-db-config
branch
from
b8d53a3 to
f153b49
Compare
|
Rebased and squashed with the new |
3 participants
Based on #72